Skip to content

Replication - #153

Closed
ritch wants to merge 40 commits into
masterfrom
feature/replication
Closed

Replication#153
ritch wants to merge 40 commits into
masterfrom
feature/replication

Conversation

@ritch

@ritch ritch commented Jan 26, 2014

Copy link
Copy Markdown
Member

Please read this document for background on our approach to replication.

http://docs.strongloop.com/display/DOC/Replication

This includes two new models

  • Change
  • Checkpoint

Here is an example app using the api.

/to @raymondfeng
/cc @bajtos

Comment thread lib/models/change.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a developer consuming the API, I would expect Change.track to run continuously. Perhaps Change.pullUpdatesOf is a better name?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the description needs to be reworded. This method rectifies change list entries for the given model ids. I'm not sure pullUpdatesOf is any less confusing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few alternate names based on your description:

  • Change.rectify (similar to Change.prototype.rectify)
  • Change.rectifyChangesOfModel
  • Change.rectifyModelChanges

@bajtos

bajtos commented Jan 27, 2014

Copy link
Copy Markdown
Member

The code looks mostly good. It's difficult to review it from the big picture point of view, as there are no test cases covering larger and/or integration scenarios.

Perhaps it's better to partition the implementation vertically, start with a full end-to-end implementation of something minimal (e.g. sync a newly created model instance) and expand horizontally later (i.e. add support for deletes, updates, conflicts - each of those can be a single self-contained change/PR).

@slnode

slnode commented Jan 28, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/850/

@ritch

ritch commented Jan 28, 2014

Copy link
Copy Markdown
Member Author

@bajtos

Perhaps it's better to partition the implementation vertically, start with a full end-to-end implementation of something minimal (e.g. sync a newly created model instance)

See https://github.com/strongloop/loopback/pull/153/files#diff-e43576aabfd6aa741af8ac58f1b4a699R673 for an example of this.

@ritch

ritch commented Jan 28, 2014

Copy link
Copy Markdown
Member Author

test please

@slnode

slnode commented Jan 29, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/852/

@slnode

slnode commented Jan 29, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/853/

@rmg

rmg commented Jan 29, 2014

Copy link
Copy Markdown
Member

test please

@slnode

slnode commented Jan 29, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/854/

@ritch

ritch commented Jan 30, 2014

Copy link
Copy Markdown
Member Author

test please

@slnode

slnode commented Jan 30, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/868/

@slnode

slnode commented Jan 30, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/870/

@slnode

slnode commented Feb 5, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/895/

@slnode

slnode commented Feb 6, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/897/

@bajtos

bajtos commented Feb 7, 2014

Copy link
Copy Markdown
Member

The example does not work:

$ node app.js
createSomeInitialSourceData
replicateSourceToTarget

current SOURCE data
 - red
 - blue
 - green


current TARGET data

updateSomeTargetData

TypeError: Cannot set property 'name' of null
    at /Users/bajtos/src/loopback/loopback/example/replication/app.js:88:16
    at Function.<anonymous> (/Users/bajtos/src/loopback/loopback/node_modules/loopback-datasource-juggler/lib/dao.js:323:5)
    at Memory.<anonymous> (/Users/bajtos/src/loopback/loopback/node_modules/loopback-datasource-juggler/lib/connectors/memory.js:107:5)
    at process._tickCallback (node.js:415:13)

@bajtos

bajtos commented Feb 7, 2014

Copy link
Copy Markdown
Member

The fact there is only one Change instance per each model instance (id) looks suspicious to me.

Will the current implementation work correctly when there are multiple replication targets, each replicating at different intervals? Consider the following timeline:

Color, Color2, Color3 - set as in example app (Color3 is setup in the same way as Color2)

0. create Color "red"
1. initial replication: Color->Color2, Color->Color3
   - a Change "color-1" of type CREATE with rev=R1 is created
2. make a change to the Color "red"
3. replicate Color -> Color2, keep Color3 outdated
   - the Change "color-1" is updated to UPDATE prev=R1 rev=R2
4. make another change to the Color "red"
5. replicate Color -> Color3
  - the Change "color-1" is updated to UPDATE prev=R2 rev=R3
  - Color3 "red" is based on rev R1, thus we have a conflict?

Am I missing something?

Comment thread lib/models/checkpoint.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of value is expected here? Is it Change.id, Model.id or something else?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it - Model.getSourceId. Could you please mention that in the property description?

@slnode

slnode commented May 16, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1200/

@slnode

slnode commented May 16, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1202/

@slnode

slnode commented May 16, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1218/

@slnode

slnode commented May 16, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1224/

creating a cache
 - Use the SharedClass class to build the remote connector
 - Change default base model from Model to DataModel
 - Fix DataModel errors not logging correct method names
 - Use the strong-remoting 1.4 resolver API to resolve dynamic remote
methods (relation api)
 - Remove use of fn object for storing remoting meta data
@slnode

slnode commented May 19, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1237/

@slnode

slnode commented May 20, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1239/

@slnode

slnode commented May 20, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1242/

@slnode

slnode commented May 20, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1244/

@slnode

slnode commented May 20, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1247/

@slnode

slnode commented May 20, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1250/

@slnode

slnode commented May 20, 2014

Copy link
Copy Markdown

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1255/

This was referenced May 21, 2014
Closed
Merged
@bajtos

bajtos commented Jun 5, 2014

Copy link
Copy Markdown
Member

@ritch what's the status of this pull request? was it already landed on the 2.0 branch?

@ritch

ritch commented Jun 5, 2014

Copy link
Copy Markdown
Member Author

Yes. Closing.

@ritch ritch closed this Jun 5, 2014
@superkhau
superkhau deleted the feature/replication branch September 30, 2016 02:02
@superkhau
superkhau restored the feature/replication branch September 30, 2016 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants